Skip to content

fix(pharmacy): prevent NPEs and over-issuing in transfer settlement#21451

Merged
buddhika75 merged 7 commits into
developmentfrom
20972-can-be-saved-with-empty-data
Jun 12, 2026
Merged

fix(pharmacy): prevent NPEs and over-issuing in transfer settlement#21451
buddhika75 merged 7 commits into
developmentfrom
20972-can-be-saved-with-empty-data

Conversation

@buddhika75

@buddhika75 buddhika75 commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary

  • Fix NPE in settle() when actual stock is below the requested quantity
  • Remove item name from insufficient-stock error message to prevent NPE when item is unavailable
  • Re-validate stock against the DB at settlement time to prevent over-issuing when stock changes after page load
  • Load the full ItemBatch before populating staff Stock metadata so transfer-issue stock rows no longer get UNKNOWN/empty metadata

Closes #20972

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Prevented settlement when current stock is missing or insufficient, and blocked issuing quantities that exceed outstanding requests.
    • Improved stock availability checks to use the absolute issued quantity for accurate calculations.
    • Ensured stock movement processing uses fully populated item/batch details to maintain data consistency.

buddhika75 and others added 4 commits June 10, 2026 09:43
…tity

When a bill item's available stock was less than the requested quantity,
the insufficient-stock error message navigated through a minimal ItemBatch
object (built from DTO data without the Item relationship populated) to
retrieve the item name, causing a NullPointerException:

  bi.getPharmaceuticalBillItem().getItemBatch().getItem().getName()
                                                  ^^^^^^^^^^^ null

Root cause: createBillItemFromStockDTO() constructs a minimal ItemBatch
from bulk-query DTO data (id, batchNo, rates, expiry) but never calls
batch.setItem(...), leaving the item field null.

Fix: use bi.getItem().getName() instead — BillItem.item is always set
correctly via newBillItem.setItem(referenceItem.getItem()) during bill
item creation and is updated by replaceSelectedSubstitute() when a
substitute is chosen.

Diagnosed from server log: NullPointerException at
TransferIssueForRequestsController.settle(TransferIssueForRequestsController.java:422)

Closes #20972

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ent NPE

The previous fix replaced getItemBatch().getItem().getName() with
bi.getItem().getName(), but bi.getItem() or getName() can also be null
(e.g. legacy data, Vmpp/Vmp items). Remove the item name reference
entirely and use a safe generic message instead.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two independent bugs allowed a transfer issue to proceed even when actual
DB stock was insufficient:

1. Stale in-memory stock check
   The validation loop compared bi.getPharmaceuticalBillItem().getStock()
   .getStock() against qty, but that Stock object is constructed at
   page-load time by createBillItemFromStockDTO() with the available qty
   captured from the bulk DTO query. Any stock movement that occurs after
   the page loads (retail sale, concurrent issue) is invisible to this
   check. Fix: replace with a live DB lookup via StockFacade.find() for
   every item immediately before saveBill() is called, so the check always
   reflects the true current stock level.

2. Negated qty passed to isStockAvailable()
   In the post-save processing loop, qty is negated at line 831 before
   isStockAvailable() is called at line 856. The internal check
   (qty > fetchedStock.getStock()) evaluates as e.g. -10 > 2 → false,
   causing the guard to always return true regardless of actual stock.
   Fix: pass Math.abs(tmpPh.getQty()) so the comparison uses the
   magnitude of the quantity.

Also made the error messages null-safe (bi.getItem() null guard) and
added a clearer message telling the user to refresh when live stock check
fails, since the cause is stale page state.

Closes #20972

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…data

When a transfer issue is settled, the PharmaceuticalBillItem carries a
minimal ItemBatch built from a DTO — only id, batchNo, and rates are set;
the item relationship is null. Both addToStock(pbi, qty, Staff) and
deductFromStock(pbi, qty, Staff) create a new Staff Stock row when none
exists, and they attempt to populate that row's metadata from the batch:

  ItemBatch ib = pharmaceuticalBillItem.getItemBatch();
  Item i = ib != null ? ib.getItem() : null;
  if (i != null) { /* set itemName, barcode, longCode, dateOfExpire, retailsaleRate */ }
  else            { s.setItemName("UNKNOWN"); s.setBarcode(""); s.setLongCode(0L); }

Because the DTO batch has no item loaded, i is always null, so every Staff
Stock row created through a transfer issue is stored with:
  - ITEM_NAME = "UNKNOWN"
  - BARCODE   = ""
  - LONGCODE  = 0
  - DATEOFEXPIRE = null
  - RETAILSALE_RATE = 0.0   (skipped in the else branch)

This does not break stock quantity movement — the JPQL lookup key is
(itemBatch, staff) by ID — but it corrupts the metadata fields used by
stock valuation and expiry reports for in-transit staff stock.

Fix: before accessing ib.getItem(), check whether the item relation is
unloaded (null) while the batch has a valid DB id. If so, call
itemBatchFacade.find(ib.getId()) to obtain the full entity. The loaded
entity is used only for metadata population; the FK already stored on
the Stock row is unchanged. The guard (getItem() == null && getId() != null)
ensures no extra query is issued when the full entity is already loaded
(normal receive-side path where ItemBatch is eagerly fetched from DB).

Applied to both:
  - addToStock(PharmaceuticalBillItem, double qty, Staff)
  - deductFromStock(PharmaceuticalBillItem, double qty, Staff)

Closes #20972

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@buddhika75, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 11 minutes and 18 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d68ccdd9-988c-49c4-b9f0-6b4630b3ab3b

📥 Commits

Reviewing files that changed from the base of the PR and between f065f97 and 9e26712.

📒 Files selected for processing (1)
  • src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java

Walkthrough

The PR adds live database stock verification in TransferIssueForRequestsController.settle() and makes PharmacyBean reload minimal ItemBatch entities before using batch/item fields during add/deduct stock operations.

Changes

Stock Validation Reliability

Layer / File(s) Summary
Stock Facade dependency setup
src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java
StockFacade is imported and injected as an @EJB dependency to enable live stock lookups.
Live stock verification in settlement
src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java
settle() re-fetches current Stock via StockFacade per item, errors on missing batch when qty>0, errors on missing/insufficient live Stock, ensures issued qty ≤ remaining requested qty, and uses Math.abs(tmpPh.getQty()) for availability checks.
Defensive ItemBatch reloading
src/main/java/com/divudi/ejb/PharmacyBean.java
addToStock() and deductFromStock() detect id-only ItemBatch objects and re-fetch the full ItemBatch via itemBatchFacade.find() before deriving Stock metadata.

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The code changes address pharmacy stock validation and settlement logic, but the linked issue #20972 concerns validation in manage_banks.xhtml with no connection to pharmacy transfer settlement. Review whether issue #20972 correctly describes the actual problem being fixed, or whether the wrong issue was linked to this PR.
Out of Scope Changes check ⚠️ Warning All changes are focused on pharmacy stock validation and transfer settlement, which is unrelated to the linked issue about bank management form validation. The pharmacy settlement fixes appear to address a different issue than #20972; verify the correct issue is linked or document why these changes resolve the banking form validation problem.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main changes: fixing NPEs and preventing over-issuing in the pharmacy transfer settlement flow, which aligns with the actual code modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 20972-can-be-saved-with-empty-data

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…aved-with-empty-data

# Conflicts:
#	src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3277fb71b5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java (1)

804-804: 💤 Low value

Consider using Math.abs() for defensive stock comparison.

The comparison liveStock.getStock() < pbi.getQty() assumes pbi.getQty() is positive. While in the current flow this should be true (qty is negated later at line 847), using Math.abs() would be defensive and consistent with the fix at line 872.

-                    if (liveStock == null || liveStock.getStock() < pbi.getQty()) {
+                    if (liveStock == null || liveStock.getStock() < Math.abs(pbi.getQty())) {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java`
at line 804, Change the stock check to defensively compare absolute quantity
values: replace the conditional if (liveStock == null || liveStock.getStock() <
pbi.getQty()) with one that uses Math.abs(pbi.getQty()), e.g. if (liveStock ==
null || liveStock.getStock() < Math.abs(pbi.getQty())), so liveStock.getStock()
is compared to the absolute requested quantity (referencing liveStock,
getStock(), and pbi.getQty()) to match the defensive pattern used later.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java`:
- Around line 790-821: The loop over getBillItems() can NPE because
createEmptyBillItem() may leave pharmaceuticalBillItem or billItemFinanceDetails
null; add null checks: when you fetch PharmaceuticalBillItem pbi =
bi.getPharmaceuticalBillItem() guard before using pbi (e.g., if pbi == null ->
add a user error or skip this BillItem) and likewise guard
bi.getBillItemFinanceDetails() before calling getQuantity() (treat null as zero
or surface an error). Update the block that uses pbi.getItemBatch(),
pbi.getStock(), and bi.getBillItemFinanceDetails().getQuantity() (and any places
calling getRemainingQuantityForItem(bi.getReferanceBillItem())) to handle nulls
deterministically so iteration does not throw NPEs.

---

Nitpick comments:
In
`@src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java`:
- Line 804: Change the stock check to defensively compare absolute quantity
values: replace the conditional if (liveStock == null || liveStock.getStock() <
pbi.getQty()) with one that uses Math.abs(pbi.getQty()), e.g. if (liveStock ==
null || liveStock.getStock() < Math.abs(pbi.getQty())), so liveStock.getStock()
is compared to the absolute requested quantity (referencing liveStock,
getStock(), and pbi.getQty()) to match the defensive pattern used later.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ecd21b25-5eba-4bb0-b2a9-e3d68207baad

📥 Commits

Reviewing files that changed from the base of the PR and between 94b5ac5 and 3277fb7.

📒 Files selected for processing (2)
  • src/main/java/com/divudi/bean/pharmacy/TransferIssueForRequestsController.java
  • src/main/java/com/divudi/ejb/PharmacyBean.java

buddhika75 and others added 2 commits June 12, 2026 20:48
…e lines

deductFromStock() already does an atomic UPDATE...WHERE s.stock >= :qty
check (the #20138 TOCTOU-safe guard) and returns false if insufficient,
handled by the existing else branch. The userStockController.isStockAvailable()
check ran after the BillItem/PharmaceuticalBillItem were already persisted,
and on failure only zeroed tmpQty and continued - leaving a saved transfer
issue line with no corresponding stock movement. UserStockContainer
reservations are not populated in this flow, so the check added no real
protection beyond the live stock check already done earlier in settle().

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ceuticalBillItem

createEmptyBillItem() (used in generateBillComponent when an item has no
available stock) adds a BillItem with pharmaceuticalBillItem left unset.
Both the live stock validation loop and the zero-qty filter in settle()
dereferenced getPharmaceuticalBillItem() unconditionally, NPEing whenever
such an item was present in the transfer issue.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@buddhika75 buddhika75 merged commit 53360f7 into development Jun 12, 2026
3 checks passed
@buddhika75 buddhika75 deleted the 20972-can-be-saved-with-empty-data branch June 12, 2026 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can be saved with empty data

1 participant